-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(levm): opcodes revert invalid selfdestruct #946
Conversation
This reverts commit 49fc47b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left some suggestions
let target_account = match self.db.accounts.get_mut(&target_address) { | ||
Some(acc) => acc, | ||
None => { | ||
// I'm considering that if address is not in the database, it means that it is an empty account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a method account_is_empty
or similar to the db implementation that returns a boolean whether with a given target address self.db.accounts.get_mut(&target_address)
.is_none()
or !.is_none()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
I did something like:
pub fn account_is_empty(&self, address: &Address) -> bool {
!self.accounts.contains_key(address)
}
self.db.accounts.insert(target_address, Account::default()); | ||
self.db.accounts.get_mut(&target_address).unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can insert the account with the balance directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
let current_account_balance = self | ||
.db | ||
.accounts | ||
.get(¤t_call_frame.to) | ||
.unwrap() | ||
.balance; | ||
self.db | ||
.accounts | ||
.get_mut(¤t_call_frame.to) | ||
.unwrap() | ||
.balance = U256::zero(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle these unwraps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the new PR that implements Database changes the db
works differently and if an account "doesn't exist" then it will return an empty account so there will be no need to handle this kind of errors. Maybe after merging that one to main I can make changes to this PR so that it fits the new database implementation.
Also, I will have to access Cache first, and if data is not cached then I will make a request to the database. That will also let me implement gas cost for cold and warm addresses.
Account::default().with_balance(current_account_balance), | ||
); | ||
} else { | ||
let target_account = self.db.accounts.get_mut(&target_address).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is looking good. Left some comments.
Motivation
Implement opcodes
This PR is going to be resumed when Db changes are merged to main.
Description
Caveat:
Closes #535
Closes #536
Closes #537